feat(spanner): add setMetricsProjectId to fix metrics export#13241
feat(spanner): add setMetricsProjectId to fix metrics export#13241RRap0so wants to merge 2 commits into
Conversation
…hared VPC On GKE with shared VPC, SpannerOptions.getProjectId() defaults to the host project via the metadata server. While DatabaseId correctly routes database operations to the application project, the client-side metrics exporter uses getProjectId() — causing createServiceTimeSeries to target the wrong project and fail with permission errors. Changes: - Add setMetricsProjectId(String) to SpannerOptions.Builder - Add resolveMetricsProjectId() that prefers explicit value, falls back to getProjectId() - Update 3 metrics export call sites to use resolveMetricsProjectId() - Log WARNING in getDatabaseClient() when DatabaseId project differs from metrics project
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
RRap0so
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to explicitly configure a project ID for client-side metrics export via SpannerOptions, addressing potential permission issues in environments like GKE with shared VPCs. The changes include adding a metricsProjectId field to the SpannerOptions builder and updating metrics initialization to use this resolved project ID. Feedback suggests moving the project mismatch warning in SpannerImpl to the client initialization block to avoid excessive logging on every getDatabaseClient call and ensuring that equals() and hashCode() in SpannerOptions are updated to include the new field.
| SpannerOptions opts = getOptions(); | ||
| String metricsProject = opts.resolveMetricsProjectId(); | ||
| if (opts.isEnableBuiltInMetrics() | ||
| && metricsProject != null | ||
| && !metricsProject.equals(db.getInstanceId().getProject())) { | ||
| logger.log( | ||
| Level.WARNING, | ||
| "DatabaseId project ''{0}'' differs from the project used for client-side metrics" | ||
| + " export ''{1}''. Metrics will be exported to ''{1}'', which may cause" | ||
| + " permission errors. Set SpannerOptions.Builder.setMetricsProjectId(\"{0}\")" | ||
| + " to fix this.", | ||
| new Object[] {db.getInstanceId().getProject(), metricsProject}); | ||
| } |
There was a problem hiding this comment.
The warning for project ID mismatch is currently logged on every call to getDatabaseClient. Since this method is frequently called during normal operation, this can lead to excessive log spam and unnecessary performance overhead. It is recommended to move this check inside the conditional block where a new DatabaseClient is actually created (i.e., when !dbClients.containsKey(db)). This ensures the warning is logged at most once per DatabaseId for the lifetime of the Spanner instance.
| private final boolean enableExtendedTracing; | ||
| private final boolean enableEndToEndTracing; | ||
| private final String monitoringHost; | ||
| private final String metricsProjectId; |
There was a problem hiding this comment.
When adding a new configuration field like metricsProjectId to SpannerOptions, it is essential to update the equals() and hashCode() methods to include it. Failure to do so can cause issues when SpannerOptions instances are used as keys in caches or when comparing configurations for equality.
Address review feedback: - Move project mismatch log inside the new-client branch so it fires at most once per DatabaseId, not on every getDatabaseClient - equals/hashCode: SpannerOptions inherits from ServiceOptions and does not override these methods; monitoringHost follows the same pattern
2 participants
Fixes #13240
Problem
On GKE with shared VPC,
SpannerOptions.getProjectId()defaults to the host project via the metadata server. WhileDatabaseIdcorrectly routes database operations to the application project, the client-side metrics exporter usesgetProjectId()— causingcreateServiceTimeSeriesto target the wrong project and fail with permission errors.Changes
setMetricsProjectId(String)toSpannerOptions.BuilderresolveMetricsProjectId()that prefers explicit value, falls back togetProjectId()resolveMetricsProjectId()WARNINGingetDatabaseClient()whenDatabaseIdproject differs from metrics projectUsage
When not set, behavior is unchanged (uses
getProjectId()).